Skip to content

Fix opcache.huge_code_pages #19388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix opcache.huge_code_pages #19388

wants to merge 1 commit into from
+114 −76

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Aug 6, 2025 •
edited
Loading

Building opcache into the main executable breaks opcache.huge_code_pages, as we were relying on the fact that accel_remap_huge_pages() is not in the same mapping as the main text segment.

Here I ensure that accel_remap_huge_pages() is placed out of the text segment, and remap only the text segment. This approach is used in https://github.com/intel/iodlr/blob/676bb7dec378d561e4d900fbdaed6ce8dbe449a1/large_page-c/large_page.c#L260.

This should work on the same systems supported by the original opcache.huge_code_pages implementation (Linux and FreeBSD).

I've also tried using mremap() in #19379, but it's not supported on FreeBSD. We could still use the mremap() approach on Linux.

Windows failure unrelated.

@arnaud-lb arnaud-lb marked this pull request as ready for review August 6, 2025 13:12
@arnaud-lb arnaud-lb requested a review from dstogov as a code owner August 6, 2025 13:12
@arnaud-lb arnaud-lb mentioned this pull request Aug 7, 2025
Closed
dstogov
dstogov approved these changes Aug 18, 2025
View reviewed changes
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood your changes in accel_remap_huge_pages(). They are fine.

I didn't completely understand the reason of changes in accel_find_program_section().
What is the problem with the original version?
I don't care a lot, I just like to know.

@arnaud-lb
Copy link
Member Author

I made this change because /proc/self/maps reflects program segments, not sections. Each segment may contain multiple sections. The segment / memory mapping containing the .text section may also contains .plt, .init, .fini, as well as the section where I placed accel_remap_huge_pages (.remap_stub), and other unspecified sections.

accel_find_program_section() finds the exact start/end of the .text section only.

@dstogov
Copy link
Member

dstogov commented Aug 18, 2025

accel_find_program_section() finds the exact start/end of the .text section only.

OK.
Will it miss read-only data section? I'm not sure how it's represented in Elf. Original version remapped it to huge-pages together with .text out of the box.

@dstogov
Copy link
Member

dstogov commented Aug 18, 2025

I understood why you need this :) - to exclude .remap_stub.

@arnaud-lb
Copy link
Member Author

I understood why you need this :) - to exclude .remap_stub.

Yes :) This also avoids problems when accel_remap_huge_pages() calls code from other sections, such as .plt.

accel_find_program_section() finds the exact start/end of the .text section only.

OK. Will it miss read-only data section? I'm not sure how it's represented in Elf. Original version remapped it to huge-pages together with .text out of the box.

Oh, I didn't notice that. I will check if I can also remap the ro data section.

@arnaud-lb
Copy link
Member Author

@dstogov I've checked, and I don't think this used to remap the read-only data section. The orignal code finds the first read-execute memory mapping whose name corresponds to __progname, and moves it to huge pages, but this mapping doesn't include .rodata. We could update the new code so that it also remaps .rodata, but this section is smaller than a huge page (at least on my system) so we would end up not remapping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dstogov dstogov dstogov approved these changes

Assignees
No one assigned
Labels
ABI break Category: Engine Extension: opcache
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arnaud-lb @dstogov